Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SAK-50500 LTI rename basiclti to lti #12902

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

csev
Copy link
Contributor

@csev csev commented Sep 20, 2024

This is two commits to make sure that git does not get confused w.r.t. the renames and text changes in a single commit. So don't squash these when merging to give us the best chance of tracking chances across the name changes.

You might find it easier to review the two commits separately. The second commit is all file changes - they first commit is merely renames in order to give us the best chance at history carryover.

This branch is running on dev1.sakaicloud.com is you want to test it on an actual system - this is the partner test server so clean up after yourself and don't break any one else's test setups.

This is aimed at Sakai 25.0. We will not merge this into master until we have done some testing on the branch. The target for merging this is 24-Sep-2024.

@csev csev added the use rebase+merge When merging, use the 'Rebase & Merge' feature to preserve authorship of commits label Sep 20, 2024
@csev csev marked this pull request as draft September 20, 2024 14:06
@jonespm
Copy link
Contributor

jonespm commented Sep 22, 2024

I checked this out and it looks like it runs as expected. There are still a lot of "BASICLTI" references in the code which we might cleanup on a separate issue after this is merged rather than trying to fix it all here. Of these I think only 1 needs to be updated, but that's easy. 2 would be nice to have. 3 sounds like a large project.

  1. On the README.md it refers to documents like these, but it looks like these were all renamed
20:* [Sakai API Documentation Including API Extensions](docs/sakai_basiclti_api.md)
22:* [Configuring the Sakai External Tools Portlet](docs/sakai_basiclti_portlet.md)
24:* [Configuring the Sakai LTI Provider](docs/sakai_basiclti_provider)
26:* [Documentation for Vendors of Sakai Tools](docs/sakai_basiclti_vendor.md)
  1. All of the variables in lti/lti-common/src/java/org/sakaiproject/lti/util/SakaiLTIUtil.java are still BASICLTI_

  2. The tool_id is still sakai.basiclti. To rename this would require changing a lot of things

    • All the names in the config/localization bundles
    • All the value\ in the sitestats Events
    • Likely a database conversion
    • A check in portal
    • The value in the help documentation which is in screensteps
    • Webservices in Sakai Script
    • Icons in the library
    • Some code in lessons would need to change to reference this like

(This might be a separate issue)
4. The values in CC are called "cartridge_basiclti" link and under an imsbasiclti name space. Some of these might not need to change but some of them are wrong and maybe a different issue. It looks like in CC 1.3 they also dropped the "basic" but it's in the for older cartridges.

Lessons has

lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/cc/Ns.java
226:    resourceTypes.put("imsbasiclti_xmlv1p3", BLTI);

But that should be imslticc_v1p3 for 1.3 and NS there doesn't exist. I'm not sure where else that is being used and maybe the export needs to be updated to this version? I don't remember if Sakai 100% supports IMS CC 1.3 or not, but looks like there was some support.

Copy link
Contributor

@jonespm jonespm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll approve this as it looks like it works as it is. These changes I suggested could be done here or in another issue/pr.

@csev
Copy link
Contributor Author

csev commented Sep 23, 2024

@jonespm Thanks for the review. I will definitely do 1 and 2. Issue 3 is very much on purpose and would be another much later JIRA - I wanted to avoid any DB conversion on this JIRA. I don't want to fix 4 in this JIRA - lets make it another JIRA - the switch in IMS world makes things a little trickier and needs to be thought of as not just a "rename" in our code.

@csev csev marked this pull request as ready for review September 23, 2024 14:47
@ern
Copy link
Contributor

ern commented Sep 23, 2024

this should not be merged until #12843 has been merged

@csev
Copy link
Contributor Author

csev commented Sep 24, 2024

So @ern I agree to delay until after until #12843 has been merged - it is way easier to redo this (because I have scripts) than to redo that PR. Any sense of an ETA?

@ern ern changed the title SAK-50500 Rename basiclti->lti SAK-50500 LTI rename basiclti to lti Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
use rebase+merge When merging, use the 'Rebase & Merge' feature to preserve authorship of commits
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants